improve error message in ridesx flashing#543
Conversation
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTightened OCI URL detection and validation: only Changes
Sequence Diagram(s)(Skipped — changes are validation/UX improvements without a new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py`:
- Around line 423-429: The current guard only checks
self._looks_like_partition_spec(path) and misses non-OCI positional inputs
(e.g., boot_a:boot.img, ./boot_a.simg) that should trigger the "missing -t"
error in multi-target mode; update the condition in the argument-parsing code
around that block to raise the ClickException whenever the positional path is
not an OCI reference (use whatever project helper detects OCI refs, e.g., an
existing is_oci_reference/_is_oci_reference function or a small url/regex check)
OR self._looks_like_partition_spec(path) or the value resembles a local
filesystem image (contains '/' or '\' or ends with common image extensions or
os.path.exists(path)); reference self._looks_like_partition_spec and the
OCI-detection helper when making the change so non-OCI inputs are caught and the
same explanatory ClickException is raised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b77aa05a-ba91-41d6-b586-e27943c3b87e
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Outdated
Show resolved
Hide resolved
bennyz
force-pushed
the
improve-ridesx-error
branch
from
ab7546a to
9db6069
Compare
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: improve error message in ridesx flashing
Fixes #540 — the approach is sound and test coverage is good. A few observations:
1. _looks_like_partition_spec misses bare filenames after colon
_looks_like_partition_spec("boot_a:boot.img") returns False because boot.img doesn't start with /, ./, ../, or ~. In flash() (single-file mode without target), this falls through to the generic error:
"This driver requires a target partition for non-OCI paths."
...which is less helpful than the new partition:path hint. The most common case (./) is covered, but bare filenames like boot_a:boot.img slip through.
Note: in _execute_flash_command (multi-target mode) this is handled correctly because the guard is not self._is_oci_path(path). The gap is only in flash().
2. Inconsistent exception types
flash()raisesValueErrorfor partition:path input_execute_flash_command()raisesclick.ClickExceptionfor similar input
click.ClickException formats cleanly to the terminal; ValueError will show as a raw traceback unless caught higher up. Since both are CLI-facing paths, they should probably both raise click.ClickException.
3. Example in multi-target error only shows one target spec
f" j storage flash -t {path} -t {next(iter(target_specs))}"If the user has 3+ -t specs the example only shows one, which may confuse them. Consider joining all specs:
t_flags = " ".join(f"-t {s}" for s in target_specs)
f" j storage flash -t {path} {t_flags}"4. Duplicated path-detection logic in driver vs client
_validate_oci_url (driver) manually re-implements the after_colon.startswith(...) check that _looks_like_partition_spec (client) also does. Architecturally acceptable given they're separate classes, but worth a comment noting the intentional duplication.
5. _is_oci_path passes host:port/image-style refs
A URL like localhost:5000/myimage passes _is_oci_path — ":" in path, "/" in path, doesn't start with excluded prefixes, and after_colon (5000/myimage) doesn't start with a path char. Probably fine in practice, but worth noting as a known edge case.
Overall: the core fix is correct and the main scenarios from #540 are addressed. Items 2 and 3 above are the most worth fixing before merge.
| _, after_colon = path.split(":", 1) | ||
| if after_colon.startswith(("/", "./", "../", "~")): | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Edge case: host:port/image passes as OCI
A URL like localhost:5000/myimage would pass this check — ":" in path, "/" in path, doesn't start with any excluded prefix, and after_colon (5000/myimage) doesn't start with a path character. Worth documenting as a known limitation, or guard with after_colon.isdigit() to reject host:port patterns.
There was a problem hiding this comment.
I suspect that's intended, seems like a container image path.
| # Traditional single file mode | ||
| if target is None: | ||
| # Detect partition:path patterns and give a specific hint | ||
| if isinstance(path, str) and self._looks_like_partition_spec(path): |
There was a problem hiding this comment.
_looks_like_partition_spec misses bare filenames after the colon
_looks_like_partition_spec("boot_a:boot.img") returns False because boot.img doesn't start with /, ./, ../, or ~. So flash("boot_a:boot.img") (without target) falls through to the generic error below instead of hitting this helpful hint. Consider also matching values where after_colon has no path separator at all but looks like a filename (e.g. ends with .img, .simg, .bin).
| # Detect partition:path patterns and give a specific hint | ||
| if isinstance(path, str) and self._looks_like_partition_spec(path): | ||
| partition, filepath = path.split(":", 1) | ||
| raise ValueError( |
There was a problem hiding this comment.
Inconsistent exception type: ValueError vs click.ClickException
This raises ValueError, but the analogous guard in _execute_flash_command raises click.ClickException, which formats cleanly in the terminal without a traceback. A ValueError here will produce a raw traceback unless caught higher up. Suggest using click.ClickException here too for consistent UX.
There was a problem hiding this comment.
This is fine, exceptions for flash() are not only meant for CLI.
| if ":" not in value: | ||
| return False | ||
| _, after_colon = value.split(":", 1) | ||
| return after_colon.startswith(("/", "./", "../", "~")) |
There was a problem hiding this comment.
Intentional duplication — worth a comment
The after_colon.startswith(...) logic here is mirrored in driver.py's _validate_oci_url. A brief note explaining the duplication is intentional (client and driver are separate classes) would help future maintainers remember to update both if the set of path prefixes ever changes.
| f"'{path}' is not an OCI reference and is missing the -t flag.\n" | ||
| f"Each image must be passed as partition:path with its own -t.\n\n" | ||
| f"Example:\n" | ||
| f" j storage flash -t {path} -t {next(iter(target_specs))}" |
There was a problem hiding this comment.
Example only shows one of the target specs
If the user has 3+ -t specs, only one appears in the suggestion, which may leave them confused. Consider including all:
t_flags = " ".join(f"-t {s}" for s in target_specs)
f" j storage flash -t {path} {t_flags}"| hint = "" | ||
| if ":" in oci_url: | ||
| _, after = oci_url.split(":", 1) | ||
| if after.startswith(("/", "./", "../", "~")): |
There was a problem hiding this comment.
Duplicated path-detection logic
This after.startswith(("/", "./", "../", "~")) check mirrors _looks_like_partition_spec in client.py. Since these live in separate classes (client vs driver) the duplication is acceptable, but a comment here noting the parallel would help keep them in sync if the set of path prefixes changes.
|
I changed the approach, looks like this now: jumpstarter⚡qti-snapdragon-ride4-sa8775p-03➤ uv run j storage flash -t system_a:./system_a.simg boot_a:./boot_a.simg
Error: 'boot_a:./boot_a.simg' is missing the -t flag.
Each partition mapping needs its own -t flag.
Example:
j storage flash -t boot_a:./boot_a.simg -t system_a:./system_a.simg
For OCI images, use the oci:// prefix:
j storage flash oci://registry.com/image:tag -t system_a:./system_a.simg
jumpstarter⚡qti-snapdragon-ride4-sa8775p-03➤ uv run j storage flash quay.io/bzlotnik/ridesx-image:ai
Error: OCI URLs must start with oci://, got: quay.io/bzlotnik/ridesx-image:ai
Usage: j storage flash oci://quay.io/bzlotnik/ridesx-image:ai |
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
improve-ridesx-error
branch
from
715863f to
3128c4b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.py (1)
97-133: Consider parameterizing the repeated invalid-input matrix.The same assertion pattern is repeated across multiple inputs in Line 99–Line 132; a parameterized test would keep this easier to extend.
Refactor sketch
+@pytest.mark.parametrize( + ("image", "targets"), + [ + ("boot_a:/path/to/boot.img", ("system_a:/path/to/system.img",)), + ("boot_a:./boot_a.simg", ("system_a:./system_a.simg",)), + ("boot_a:boot.img", ("system_a:/path/to/system.img",)), + ("./boot_a.simg", ("system_a:/path/to/system.img",)), + ("quay.io/org/image:tag", ("boot_a:boot.img",)), + ], +) +def test_execute_flash_command_rejects_non_oci_positional_with_targets( + ridesx_client, image, targets +): + with pytest.raises(click.ClickException, match="missing the -t flag"): + ridesx_client._execute_flash_command(image, targets) - -def test_execute_flash_command_rejects_non_oci_positional_with_targets(ridesx_client): - ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.py` around lines 97 - 133, The test test_execute_flash_command_rejects_non_oci_positional_with_targets repeats the same pytest.raises assertion for multiple invalid inputs; refactor it to a single parametric test using pytest.mark.parametrize that iterates over the invalid positional values and calls ridesx_client._execute_flash_command with each value and a representative second-target tuple, asserting a click.ClickException with the "missing the -t flag" match; keep the same test name (or rename clearly) and include the original cases (e.g., "boot_a:/path/to/boot.img", "boot_a:./boot_a.simg", "boot_a:boot.img", "./boot_a.simg", "quay.io/org/image:tag") as the parameter list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.py`:
- Around line 97-133: The test
test_execute_flash_command_rejects_non_oci_positional_with_targets repeats the
same pytest.raises assertion for multiple invalid inputs; refactor it to a
single parametric test using pytest.mark.parametrize that iterates over the
invalid positional values and calls ridesx_client._execute_flash_command with
each value and a representative second-target tuple, asserting a
click.ClickException with the "missing the -t flag" match; keep the same test
name (or rename clearly) and include the original cases (e.g.,
"boot_a:/path/to/boot.img", "boot_a:./boot_a.simg", "boot_a:boot.img",
"./boot_a.simg", "quay.io/org/image:tag") as the parameter list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46079594-2993-4202-943d-ebc57ec26466
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client_test.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.pypython/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver.py
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/driver_test.py
- python/packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Remove the _is_oci_path heuristic and _looks_like_partition_spec methods. OCI references now require the oci:// prefix, matching what FLS fastboot expects downstream. Improve error messages to distinguish between missing -t flags and missing oci:// prefixes based on path structure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
improve-ridesx-error
branch
from
3128c4b to
d8017f4
Compare
2 participants
fixes #540